Skip to content

Add namespace label selector support for reflection#620

Merged
winromulus merged 11 commits intoemberstack:mainfrom
davidswimbird:davidswimbird/discussion-409
Apr 24, 2026
Merged

Add namespace label selector support for reflection#620
winromulus merged 11 commits intoemberstack:mainfrom
davidswimbird:davidswimbird/discussion-409

Conversation

@davidswimbird
Copy link
Copy Markdown
Contributor

@davidswimbird davidswimbird commented Mar 25, 2026

Add two new annotations that allow selecting target namespaces by
Kubernetes label selectors instead of only name regex patterns:

  • reflection-allowed-namespaces-selector
  • reflection-auto-namespaces-selector

When both a name-pattern and a label selector annotation are set, a
namespace matches if it satisfies either condition (OR logic). The label
selector supports standard Kubernetes syntax: equality (=, ==, !=),
existence, and set-based (in, notin) expressions.

This is useful when you want to reflect a secret to a subset of non-deterministic namespaces, that can't be represented with a regex.

For my use case, it makes the manual overhead of managing reflector a lot smaller, as I can automate the label configuration for my namespaces ahead of time, to know that they will be receiving the secrets when the namespace is created with a specified label.

This is based on the suggestion in: Discussion #409

Add two new annotations that allow selecting target namespaces by
Kubernetes label selectors instead of only name regex patterns:
- reflection-allowed-namespaces-selector
- reflection-auto-namespaces-selector

When both a name-pattern and a label selector annotation are set, a
namespace matches if it satisfies either condition (OR logic). The label
selector supports standard Kubernetes syntax: equality (=, ==, !=),
existence, and set-based (in, notin) expressions.

Closes emberstack#409
@davidswimbird davidswimbird force-pushed the davidswimbird/discussion-409 branch from a75fa1c to a704f21 Compare March 25, 2026 10:42
@winromulus
Copy link
Copy Markdown
Contributor

@davidswimbird while I review this, please update the documentation as well (readme)

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds namespace label selector support to the reflector’s “allowed namespaces” and “auto namespaces” logic, enabling selection of target namespaces via standard Kubernetes label selector syntax in addition to existing name/regex matching.

Changes:

  • Introduces two new reflection annotations for label selectors (allowed/auto namespaces selectors) and wires them into MirroringProperties.
  • Implements label-selector matching (with OR logic vs name-pattern matching) and updates mirroring flows to use V1Namespace when available.
  • Adds unit tests and test helpers for selector behavior; exposes internals to the test project.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
tests/ES.Kubernetes.Reflector.Tests/LabelSelectorMatchTests.cs Adds unit tests for label selector parsing/matching and OR logic with name patterns.
tests/ES.Kubernetes.Reflector.Tests/Additions/ReflectorAnnotationsBuilder.cs Adds builder helpers for the new selector annotations in tests.
src/ES.Kubernetes.Reflector/Mirroring/Core/ResourceMirror.cs Caches namespaces for selector evaluation and uses V1Namespace overloads for matching.
src/ES.Kubernetes.Reflector/Mirroring/Core/MirroringPropertiesExtensions.cs Parses new annotations, adds selector-aware matching, and implements label selector evaluation.
src/ES.Kubernetes.Reflector/Mirroring/Core/MirroringProperties.cs Adds properties for allowed/auto namespace label selectors.
src/ES.Kubernetes.Reflector/Mirroring/Core/Annotations.cs Adds constants for the new selector annotations.
src/ES.Kubernetes.Reflector/ES.Kubernetes.Reflector.csproj Adds InternalsVisibleTo to allow unit tests to access LabelSelectorMatch.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/ES.Kubernetes.Reflector/Mirroring/Core/MirroringPropertiesExtensions.cs Outdated
Comment thread src/ES.Kubernetes.Reflector/Mirroring/Core/ResourceMirror.cs Outdated
Comment thread tests/ES.Kubernetes.Reflector.Tests/LabelSelectorMatchTests.cs
Comment thread src/ES.Kubernetes.Reflector/Mirroring/Core/MirroringPropertiesExtensions.cs Outdated
Document the new reflection-allowed-namespaces-selector and
reflection-auto-namespaces-selector annotations from emberstack#620.
Invalid or degenerate selectors (e.g. ",", "!", "!=value") previously
produced no valid requirements or empty keys, causing LabelSelectorMatch
to return true and unintentionally allow reflection to all namespaces.
Now returns false when no requirements are parsed or a key is empty.
The namespace cache was only populated on Added events, so label changes
(Modified) left stale entries and deleted namespaces were never evicted.

- Handle Modified: update cache and reconcile auto-reflections (create
  if now matching, delete if no longer matching)
- Handle Deleted: evict cache entry and clean up auto-reflection tracking
Cover unbalanced parentheses, empty set-based expressions, and bare
operators to document that the parser fails closed on all of these.
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 9, 2026

Automatically marked as stale due to no recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions Bot added the stale label Apr 9, 2026
@davidswimbird
Copy link
Copy Markdown
Contributor Author

Hey @winromulus. How's the review coming along? :)

@github-actions github-actions Bot removed the stale label Apr 10, 2026
@winromulus
Copy link
Copy Markdown
Contributor

@davidswimbird a couple of comments left as pending. Can you check those please?

@davidswimbird
Copy link
Copy Markdown
Contributor Author

@winromulus resolved all comments. They had already been adressed, just didn't want to resolve comments without you having a look :)

Copy link
Copy Markdown
Contributor

@winromulus winromulus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey — thanks for your patience with this one, sorry it sat longer than it should have. Circling back now with a proper pass.

First, the Copilot round was handled really well. I checked each one against current code: the fail-closed path in LabelSelectorMatch, the Modified + Deleted handlers, the negative-case tests, and the updated XML doc are all solid. The negative test coverage in particular (the InvalidSelector_* theories) is exactly the right shape.

I had a couple of items from an earlier review pass that didn't come through on the PR conversation — apologies for that, they're on me. I've restated them as inline comments below along with a few smaller things I spotted on this pass.

On tests: the unit coverage for the parser is thorough, but there's no end-to-end coverage for the feature itself. Given the existing Testcontainers.K3s pattern (see MirroringIntegrationTests.AutoReflect_To_AllowedNamespaces), it would be worth one or two integration tests — particularly one that exercises the new Modified-path logic (label changes creating/removing reflections), since that's easy to silently break with a future refactor.

Thanks again for the contribution — the core approach is sound and the implementation is genuinely considerate (the XML docs, the overload preservation, the InternalsVisibleTo pattern, all good calls). Happy to merge once the items below are resolved.

Comment thread src/ES.Kubernetes.Reflector/Mirroring/Core/ResourceMirror.cs Outdated
Comment thread src/ES.Kubernetes.Reflector/Mirroring/Core/ResourceMirror.cs
Comment thread src/ES.Kubernetes.Reflector/Mirroring/Core/ResourceMirror.cs
Comment thread src/ES.Kubernetes.Reflector/Mirroring/Core/MirroringPropertiesExtensions.cs Outdated
…gured

When a source configures only a label selector (no name pattern) and the
target namespace isn't in _namespaceCache (e.g. after a watcher reconnect
or at startup before namespace events arrive), the string overload of
CanBeReflectedToNamespace fell through to PatternListMatch("", ns),
which returns true for empty patterns — silently allowing reflection to
every namespace.

Deny reflection on cache miss when a label selector is configured so the
selector cannot be bypassed by missing cache state.
Adds two Testcontainers.K3s integration tests covering the new
reflection-allowed-namespaces-selector annotation:

- AutoReflect_To_NamespacesMatchingLabelSelector: source with a label
  selector reflects only to namespaces whose labels match.

- AutoReflect_UpdatesReflections_WhenNamespaceLabelsChange: exercises
  the namespace Modified-path so that relabeling an existing namespace
  creates or removes the auto-reflection accordingly. This covers logic
  that is easy to silently break with a future refactor.

Extends BaseIntegrationTest with a labels argument on CreateNamespaceAsync
and a resilience pipeline that waits for a resource to become absent.
Previously the label selector parser accepted any characters matching a
loose regex as a key, and malformed selectors silently produced no match
with no feedback to the operator — a typo in an annotation looked like
"nothing reflected".

This change:

- Refactors the parser to produce a V1LabelSelector and a list of parse
  errors, separating parsing from evaluation. Matching now uses the
  standard Kubernetes operator semantics (In, NotIn, Exists, DoesNotExist).
- Adds strict validation for label keys (optional DNS-subdomain prefix up
  to 253 chars, name segment up to 63 chars, start/end alphanumeric) and
  label values, matching the constraints Kubernetes itself enforces.
- Exposes MirroringProperties.GetLabelSelectorErrors() so callers can
  surface parse errors.
- In ResourceMirror.HandleUpsert, logs a warning for each parse error the
  first time a given signature is observed for a source (and when it
  changes), so operators get signal for misconfigured annotations without
  log spam on repeated watch events.

LabelSelectorMatch still fails closed on invalid input; existing behavior
is preserved for all previously-supported valid selectors.
Mirror auto-reflection's namespace event handling for direct reflections so
selector changes take effect immediately rather than waiting for the next
source upsert to prune the cache. Also clear direct cache entries on namespace
deletion to match the existing auto cleanup.
Namespace Modified events fire on every status, annotation, or
resourceVersion bump. Reflection eligibility only depends on the
namespace name and labels, so short-circuit the auto-source loop and
direct-reflection rebalance when the cached namespace's labels match
the incoming ones.
@davidswimbird davidswimbird force-pushed the davidswimbird/discussion-409 branch from ba60e77 to baf0a2d Compare April 23, 2026 07:46
@davidswimbird
Copy link
Copy Markdown
Contributor Author

Thanks for the review @winromulus! Appreciate that you took the time and dug deep, you found things I would never have caught myself.

I've adressed your inline comments, and added integration tests for the full feature as per the root review comment.

Let me know if there's anything else I should adress :)

Copy link
Copy Markdown
Contributor

@winromulus winromulus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really thorough response — appreciate the care on every point. I walked through each of the new commits against the earlier asks and they all hold up.

  • Security fallback (983e507): CanBeReflectedToNamespaceCached / CanBeAutoReflectedToNamespaceCached now fail closed when a selector is configured and the namespace isn't cached, with a debug log for visibility. Clean, preserves existing behavior for regex-only sources.
  • Parser refactor (551bdc9): V1LabelSelector / V1LabelSelectorRequirement as the internal representation, K8s-spec label key/value validation (prefix + name, length limits, regexes), and warning logs deduped per source-signature — exactly the surface area that was missing. The split between TryParseSetBased / TryParseInequality / TryParseEquality / TryParseExistence reads cleanly and each path validates independently. GetLabelSelectorErrors as a public diagnostic is a nice touch.
  • Direct reflection rebalance (25ab8ff): both Modified and Deleted now walk _directReflectionCache, and the DirectReflect_StopsUpdating_WhenNamespaceLabelsNoLongerMatchSelector integration test exercises exactly the shift-label-mid-flight scenario.
  • Labels-equal short-circuit (baf0a2d): NamespaceLabelsEqual with empty-dict coalescing, dedicated unit tests covering null/empty/add/remove/rename/same, applied before both the auto-source loop and the direct-rebalance loop.
  • Integration tests: AutoReflect_To_NamespacesMatchingLabelSelector and AutoReflect_UpdatesReflections_WhenNamespaceLabelsChange plus the direct-reflection test give end-to-end protection for the new behavior. Pattern matches the existing AutoReflect_To_AllowedNamespaces shape, which is exactly right.

Two very small observations I won't block on:

  1. NamespaceLabelsEqual uses SequenceEqual, which is order-sensitive — two semantically-equal dicts with different insertion order would compare unequal and fall through to reconciliation. Correctness is preserved (more work, not less), so it's fine as-is; a Count + All(TryGetValue...) check would be order-independent if you want to tune it.
  2. TryParseEquality silently overwrites duplicate keys with conflicting values — e.g. env=prod,env=staging parses as matchLabels["env"] = "staging" rather than being rejected. Real-world operators won't write that, and kubectl would reject it, but a "duplicate key" check would make the parser a touch more faithful. Up to you — I'd leave it.

Really strong engagement through the review. Approving — happy to merge.

@winromulus winromulus merged commit 3f426f4 into emberstack:main Apr 24, 2026
4 checks passed
eleboucher pushed a commit to eleboucher/homelab that referenced this pull request Apr 27, 2026
… (10.0.35 → 10.0.39) (#293)

This PR contains the following updates:

| Package | Update | Change |
|---|---|---|
| [ghcr.io/emberstack/helm-charts/reflector](https://github.com/emberstack/kubernetes-reflector) | patch | `10.0.35` → `10.0.39` |

---

### Release Notes

<details>
<summary>emberstack/kubernetes-reflector (ghcr.io/emberstack/helm-charts/reflector)</summary>

### [`v10.0.39`](https://github.com/emberstack/kubernetes-reflector/releases/tag/v10.0.39)

[Compare Source](emberstack/kubernetes-reflector@v10.0.38...v10.0.39)

The release process is automated.

#### What's Changed

- Add namespace exclusion for watchers by [@&#8203;komapa](https://github.com/komapa) in [#&#8203;623](emberstack/kubernetes-reflector#623)

#### New Contributors

- [@&#8203;komapa](https://github.com/komapa) made their first contribution in [#&#8203;623](emberstack/kubernetes-reflector#623)

**Full Changelog**: <emberstack/kubernetes-reflector@v10.0.38...v10.0.39>

### [`v10.0.38`](https://github.com/emberstack/kubernetes-reflector/releases/tag/v10.0.38)

[Compare Source](emberstack/kubernetes-reflector@v10.0.37...v10.0.38)

The release process is automated.

#### What's Changed

- Add namespace label selector support for reflection by [@&#8203;davidswimbird](https://github.com/davidswimbird) in [#&#8203;620](emberstack/kubernetes-reflector#620)

#### New Contributors

- [@&#8203;davidswimbird](https://github.com/davidswimbird) made their first contribution in [#&#8203;620](emberstack/kubernetes-reflector#620)

**Full Changelog**: <emberstack/kubernetes-reflector@v10.0.37...v10.0.38>

### [`v10.0.37`](https://github.com/emberstack/kubernetes-reflector/releases/tag/v10.0.37)

[Compare Source](emberstack/kubernetes-reflector@v10.0.36...v10.0.37)

The release process is automated.

#### What's Changed

- Bump the all-dependencies group with 8 updates by [@&#8203;dependabot](https://github.com/dependabot)\[bot] in [#&#8203;642](emberstack/kubernetes-reflector#642)

**Full Changelog**: <emberstack/kubernetes-reflector@v10.0.36...v10.0.37>

### [`v10.0.36`](https://github.com/emberstack/kubernetes-reflector/releases/tag/v10.0.36)

[Compare Source](emberstack/kubernetes-reflector@v10.0.35...v10.0.36)

The release process is automated.

#### What's Changed

- Bump the all-dependencies group with 6 updates by [@&#8203;dependabot](https://github.com/dependabot)\[bot] in [#&#8203;641](emberstack/kubernetes-reflector#641)

**Full Changelog**: <emberstack/kubernetes-reflector@v10.0.35...v10.0.36>

</details>

---

### Configuration

📅 **Schedule**: Branch creation - "before 6am on Monday" in timezone Europe/Paris, Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled because a matching PR was automerged previously.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0My4xMDEuMSIsInVwZGF0ZWRJblZlciI6IjQzLjEwMS4xIiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6WyJyZW5vdmF0ZS9jb250YWluZXIiLCJ0eXBlL3BhdGNoIl19-->

Reviewed-on: https://git.erwanleboucher.dev/eleboucher/homelab/pulls/293
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants